use variable for client schemes, allowing override #467
Conversation
This change is intended to make the default client implementations more flexible so that their scheme can be customized. This can be useful in scenarios where a subclass wants to implement a custom scheme on e.g. a S3 compatible API [1] but with a custom scheme so that the default S3 access is still also available. [1] https://cloudpathlib.drivendata.org/stable/authentication/#accessing-custom-s3-compatible-object-stores The tests have been updated to include a new s3-like rig which uses the new scheme override functionality.
kujenga
force-pushed
the
at/client-prefix-flexibility
branch
from
b458e3c
to
54c37b2
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## 467-live-tests #467 +/- ##
================================================
- Coverage 94.2% 93.2% -1.0%
================================================
Files 23 23
Lines 1745 1745
================================================
- Hits 1645 1628 -17
- Misses 100 117 +17
|
There was a problem hiding this comment.
Yeah, the hard-coding is a bug. Thanks for the fix. Agreed we want to enable scenarios like you lay out.
Do we need to add cloud_prefix to the client though? I'd rather keep it in one place just on the path.
For all the instances you replaced, you should be able to get the cloud_prefix from the instance of the cloud_path passed to the method, no?
Would that work for your use cases? If not, we could potentially add cloud_prefix as a property that reaches through _cloud_meta to get the associated cloud_prefix from the path_class:
https://github.com/drivendataorg/cloudpathlib/blob/master/cloudpathlib/cloudpath.py#L96-L100
|
Also, one other thought: I think adding a rig for this is overkill. That will duplicate all the tests, which isn't necessary. I'd just do a single test with the custom scheme to show that piece works. |
kujenga
force-pushed
the
at/client-prefix-flexibility
branch
from
39aeb09
to
1b3f153
Compare
kujenga
force-pushed
the
at/client-prefix-flexibility
branch
from
672f724
to
3cf711a
Compare
Good call, done!
Fair enough, done! If there's anything else just let me know! I left them as separate commits to keep what I changed clear but happy to squash as well if you prefer. |
This isolates the implementation in response to PR feedback: drivendataorg#467 (comment)
This isolates the implementation in response to PR feedback: drivendataorg#467 (comment)
kujenga
force-pushed
the
at/client-prefix-flexibility
branch
from
9b2bf69
to
147db5b
Compare
|
great, let me know if there's anything else! |
* use variable for client schemes, allowing override This change is intended to make the default client implementations more flexible so that their scheme can be customized. This can be useful in scenarios where a subclass wants to implement a custom scheme on e.g. a S3 compatible API [1] but with a custom scheme so that the default S3 access is still also available. [1] https://cloudpathlib.drivendata.org/stable/authentication/#accessing-custom-s3-compatible-object-stores The tests have been updated to include a new s3-like rig which uses the new scheme override functionality. * use single cloud_prefix * tests: switch to lighter-weight custom scheme tests * update HISTORY file for custom scheme support * update custom scheme tests to utilize pytest fixture This isolates the implementation in response to PR feedback: #467 (comment) Co-authored-by: Aaron Taylor <[email protected]>
This change is intended to make the default client implementations more flexible so that their scheme can be customized. This can be useful in scenarios where a subclass wants to implement a custom scheme on e.g. a S3 compatible API [1] but with a custom scheme so that the default S3 access is still also available.
[1] https://cloudpathlib.drivendata.org/stable/authentication/#accessing-custom-s3-compatible-object-stores
The tests have been updated to include a new s3-like rig which uses the
new scheme override functionality.
(If there is a better strategy for tests or this seems like overkill let me know and happy to adjust! It seems a bit more work is needed to get them passing)
Closes #466
Contributor checklist:
CONTRIBUTING.mdCloses #issueappears in the PR summary (e.g.,Closes #123).HISTORY.mdwith the issue that is addressed and the PR you are submitting. If the top section is not `## UNRELEASED``, then you need to add a new section to the top of the document for your change.